Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix h_max definition #2403

Merged
merged 14 commits into from
Oct 23, 2023
Merged

Fix h_max definition #2403

merged 14 commits into from
Oct 23, 2023

Conversation

kosack
Copy link
Contributor

@kosack kosack commented Sep 21, 2023 •
edited
Loading

Fixes #2395

  • In HillasReconstructor, Define h_max as the vertical height above sea level (including the observatory height) of the shower-max point, and not the distance from the array which was there before.
  • In HillasIntersection, use the correct observatory height value

When #2402 is merged, I will remove the checks for missing reference_location

@kosack kosack marked this pull request as draft September 21, 2023 15:29
@maxnoe
Copy link
Member

maxnoe commented Sep 22, 2023

Docs failure is fixed in main, please rebase

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@kosack
Copy link
Contributor Author

kosack commented Sep 28, 2023

I think I will still add "slant" versions of the two quantities to the data model:

  • shower_distance: the distance from the impact point of the shower projected to sea-level to the shower-max point
  • shower_slant_depth: the line integral over the atmosphere from infinity to the shower-max point along the shower axis

The question now is if we keep h_max and x_max or rename them shower_height and shower_depth (breaking data model change, but as clearly nobody has used these up to now, maybe it's not a worry)

@maxnoe
Copy link
Member

maxnoe commented Sep 28, 2023

Something went wrong here with the merge/rebase?

@kosack kosack force-pushed the fix/h_max_definition branch 2 times, most recently from 41729d7 to f7a5eec Compare September 28, 2023 11:44
@kosack
Copy link
Contributor Author

kosack commented Sep 28, 2023

Something went wrong here with the merge/rebase?

not sure what it was, but I rebased again and did a force-with-lease, and seems ok now

maxnoe
maxnoe reviewed Sep 29, 2023
View reviewed changes
ctapipe/containers.py Outdated Show resolved Hide resolved
@kosack kosack marked this pull request as ready for review October 20, 2023 15:00
@maxnoe maxnoe merged commit b9b2a1a into cta-observatory:main Oct 23, 2023
12 checks passed
@kosack kosack mentioned this pull request Feb 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxnoe maxnoe maxnoe approved these changes

@Tobychev Tobychev Tobychev approved these changes

@HealthyPear HealthyPear Awaiting requested review from HealthyPear HealthyPear is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

h_max is inconsistent between HillasIntersection and HillasReconstructor
3 participants
@kosack @maxnoe @Tobychev